Generalize recipes and implement them by Limeth · Pull Request #1449 · SpongePowered/SpongeAPI

Limeth · 2017-01-02T16:37:25Z

This is a WIP continuation of kashike's PR, with his consent.
It aims to generalize how recipes are registered and handled. Instead of checking the equality of the ingredients, it works by testing ingredient predicates. It also aims to let the user implement the recipe interfaces by themselves, making it, for example, possible to transfer NBT data from the ingredients to the result.
In this PR, ItemStackSnapshots are used extensively. This is to prevent the user from modifying the original ItemStack. I could instead provide an ItemStack#copy, but that doesn't make it visually obvious the original item won't change.

Limeth · 2017-01-02T19:20:38Z

The SpongeAPI part is now in the state of what I'd like to propose. Comments and reviews very welcome. Continuing by attempting to implement the SpongeCommon part.

ST-DDT · 2017-01-02T19:33:25Z

src/main/java/org/spongepowered/api/item/recipe/crafting/ShapedCraftingRecipe.java

+         * @throws IllegalArgumentException If the aisle does not contain
+         *     the specified character symbol
+         */
+        Builder where(char symbol, @Nullable ItemStackSnapshot ingredient) throws IllegalArgumentException;


I guess this can be default implemented as well.

It cannot, the default item matching behavior in vanilla Minecraft is defined in NMS code and I needed to mimic it. Can't do that with SpongeAPI. Maybe I should create a method in SpongeCommon that mimics the behavior and expose it via SpongeAPI?

Don't accept null as ingredient. Ask users to use ItemStackSnapshot.NONE

@Limeth What part of item matching is not possible in SpongeAPI?

@gabizou This part, as far as I'm aware.

@Limeth: That link seems to be broken. Can the implementation take care of passing in ItemStackSnapshot.NONE to the predicate, if that's what the issue is?

ST-DDT · 2017-01-02T19:34:45Z

src/main/java/org/spongepowered/api/item/recipe/crafting/CraftingRecipe.java

+    /**
+     * @return {@code width * height} for shaped recipes, or the number of ingredients for shapeless recipes
+     */
+    int getRecipeSize();


When would i/the impl need this method?

The implementation on the NMS side assigns priority to recipes depending on this value. You would use this to change that priority.

Please document this behaviour in the javadocs.

ST-DDT · 2017-01-02T19:36:34Z

src/main/java/org/spongepowered/api/item/recipe/RecipeRegistry.java

+     * Removes the given CraftingRecipe from registration in this registry.
     *
-     * @param recipe The Recipe to unregister
+     * @param recipe The CraftingRecipe to unregister


The javadocs mention crafting recipes but the generic type allows plain recipes as well.

Thanks, didn't catch this one after auto-refactoring it.

ST-DDT · 2017-01-02T19:42:41Z

src/main/java/org/spongepowered/api/item/recipe/crafting/CraftingRecipe.java

+     * @return An {@link ItemStack} or {@link Optional#empty()} if the given
+     *         {@link GridInventory} does not match this recipe's requirements.
+     */
+    Optional<ItemStack> getResult(GridInventory grid);


Here you get the results but how do you actually consume the items? Is it required to reduce the number by one? How about Blood Magic's magicians orbs that act as a mere catalyst and thus is not consumed? Or recipes where a tool is used and the tool only takes some damage?

The removal of items from the crafting grid is handled by the CraftingManager in NMS. To preserve items in the crafting grid, provide them via #getRemainingItems(GridInventory).

ST-DDT · 2017-01-02T19:46:08Z

src/main/java/org/spongepowered/api/item/recipe/smelting/SmeltingRecipe.java

+     * @return Whether this {@param ingredient} can be used to craft the result
+     */
+    default boolean isValid(ItemStackSnapshot ingredient) {
+        // ItemStackSnapshot#NONE gets replaced at runtime


Missing null check.

The argument is non-null by default, as defined in the package-info.java file in the package.

Ah, turns out @Nonnull is only used for static analysis.

ST-DDT · 2017-01-02T19:47:27Z

src/main/java/org/spongepowered/api/item/recipe/crafting/ShapelessCraftingRecipe.java

+
+    @Override
+    default boolean isValid(GridInventory grid, World world) {
+        List<Predicate<ItemStackSnapshot>> predicates = Lists.newLinkedList(getIngredientPredicates());


Missing null checks.

Deamon5550 · 2017-01-02T20:00:21Z

src/main/java/org/spongepowered/api/item/recipe/crafting/ShapedCraftingRecipe.java

+     * @param y The y coordinate counted from the top
+     * @return The ingredient predicate at this position defined by the aisle
+     */
+    default Optional<Predicate<ItemStackSnapshot>> getIngredientPredicate(int x, int y) {


Why are these methods default implemented?

So people who want to implement this interface themselves don't have to implement this method.

Is that not what builders are for? if you're cluttering up the API to make it a little easier for the tiny percent of people why have to implement this in a plugin then you can remove all of these default implementations.

The only things that should be default implemented is when its a trivial helper method.

I'd find it incredibly useful to not have to write these myself, what is the issue exactly? All ShapedCraftingRecipes should behave like this, this is why they are default-implemented. And in corner cases, it also makes it possible to override them. It sounds like a win-win scenario to me. Note that all CraftingRecipes have an isValid method.

I'd much rather have the least amount of actual implementation in the API. That being said, this is implementation, not API.

kashike · 2017-01-02T20:44:04Z

src/main/java/org/spongepowered/api/item/recipe/crafting/CraftingRecipe.java

+ * or you can simply use the {@link ShapelessCraftingRecipe.Builder} or {@link ShapedCraftingRecipe.Builder}.</p>
+ */
+public interface CraftingRecipe extends Recipe {
+    /**


Missing new line after body start.

kashike · 2017-01-02T20:44:30Z

src/main/java/org/spongepowered/api/item/recipe/crafting/ShapedCraftingRecipe.java

+    }
+
+    /**
+     * @return An unmodifiable copy of the original aisle


Needs full javadocs.

kashike · 2017-01-02T20:44:49Z

src/main/java/org/spongepowered/api/item/recipe/crafting/ShapedCraftingRecipe.java

+        int gapHeight = grid.getRows() - getHeight();
+
+        // Shift the aisle along the grid wherever possible
+        for(int offsetX = 0; offsetX <= gapWidth; offsetX++) {


for ( (space)

Do I have my checkstyle plugin set up incorrectly? I am not getting the warnings here.

Maxqia · 2017-01-03T07:56:52Z

-_- #1419

Limeth · 2017-01-03T17:31:23Z

Began implementing the SpongeCommon part.

Deamon5550

Still some implementation in here, a few missing or misworded javadocs, and you need to go through with your IDE and fix the line wrapping of your javadocs its all over the place.

Deamon5550 · 2017-01-03T20:42:32Z

src/main/java/org/spongepowered/api/item/recipe/Recipe.java

-     */
-    boolean isValid(GridInventory grid);
-
    /**


Missing javadoc

Deamon5550 · 2017-01-03T20:45:14Z

src/main/java/org/spongepowered/api/item/recipe/crafting/CraftingRecipe.java

+import java.util.Optional;
+
+/**
+ * <p>A CraftingRecipe represents some craftable recipe in the game.</p>


Don't need paragraph tags for the first paragraph.

Deamon5550 · 2017-01-03T20:46:35Z

src/main/java/org/spongepowered/api/item/recipe/crafting/CraftingRecipe.java

+ * <p>The requirements of a CraftingRecipe can be general, they just have to
+ * eventually return a boolean given an itemgrid.</p>
+ *
+ * <p>You can implement it manually to suit your creative needs,


Skip this bit, its confusing and its stating the obvious.

Deamon5550 · 2017-01-03T20:48:44Z

src/main/java/org/spongepowered/api/item/recipe/crafting/CraftingRecipe.java

+     * This method is preferred over the {@link #getExemplaryResult()} method,
+     * as it customizes the result further depending on the specified
+     * input {@param grid}.
+     * It is advised to use the output of {@link #getExemplaryResult()},


Either join this to the previous line or make it a new paragraph.

Deamon5550 · 2017-01-03T20:54:03Z

src/main/java/org/spongepowered/api/item/recipe/crafting/ShapedCraftingRecipe.java

+     *
+     * @return An unmodifiable copy of the original aisle
+     */
+    List<String> getAisle();


The aisle stuff is great for the builder but it doesn't have a place in the actual recipe. You already have width and height information and a method for getting the predicate for a position, keeping the aisle stuff around is useless as translating the characters is way more work than just using the methods that take an x, y position.

Deamon5550 · 2017-01-03T20:57:18Z

src/main/java/org/spongepowered/api/item/recipe/smelting/SmeltingRecipe.java

+     * @return The result of smelting the {@param ingredient} {@link ItemStack}, if the {@param ingredient} is valid
+     */
+    default Optional<ItemStack> getResult(ItemStackSnapshot ingredient) {
+        Preconditions.checkNotNull(ingredient, "The ingredient must not be null");


Remove this implementation

Deamon5550 · 2017-01-03T20:58:08Z

src/main/java/org/spongepowered/api/item/recipe/smelting/SmeltingRecipe.java

+import java.util.Optional;
+
+/**
+ * A general interface for furnace recipes.


Whatever you are wrapping the javadocs in this whole file to it isn't 80 characters.

Deamon5550 · 2017-01-03T20:58:46Z

src/main/java/org/spongepowered/api/item/recipe/smelting/SmeltingRecipe.java

+     */
+    ItemStackSnapshot getExemplaryIngredient();
+
+    /**


Missing javadoc

Deamon5550 · 2017-01-03T20:59:45Z

src/main/java/org/spongepowered/api/item/recipe/smelting/SmeltingRecipe.java

+    }
+
+    /**
+     * This method is preferred over the {@link #getExemplaryResult()} method,


Shouldn't say preferred, this is the method to use.

Deamon5550 · 2017-01-03T20:59:59Z

src/main/java/org/spongepowered/api/item/recipe/smelting/SmeltingRecipe.java

+    }
+
+    /**
+     * @param ingredient The ingredient being smelted


Missing javadoc

Limeth · 2017-01-03T22:01:06Z

@Deamon5550 Thanks for the review. Should this be highlighted by the checkstyle plugin? I am not getting any warnings like those you named.

Deamon5550 · 2017-01-03T22:02:38Z

The checkstyle and formatter do their best to help you but common sense is always the best.

Limeth · 2017-01-04T17:14:22Z

@Deamon5550 Fixed.

ST-DDT

Nice API design. I guess this will become quite handy.

ST-DDT · 2017-01-07T11:39:02Z

src/main/java/org/spongepowered/api/item/recipe/smelting/SmeltingRecipe.java

+    /**
+     * Builds a simple furnace recipe
+     */
+    interface Builder extends ResettableBuilder<SmeltingRecipe, Builder> {


Missing javadocs for all methods inside this builder.

ST-DDT · 2017-01-07T11:46:51Z

src/main/java/org/spongepowered/api/item/recipe/crafting/CraftingRecipe.java

+     *         or {@link Optional#empty()} if the given
+     *         {@link GridInventory} does not match this recipe's requirements.
+     */
+    Optional<List<ItemStack>> getRemainingItems(GridInventory grid, World world);


I quite unsure about this one.

Wouldn't be a Optional<CraftingResult> better here?
Because currently you have to check the validity of the recipe twice (one for the result stack and one time for the remaining stacks).

If you use a CraftingResult which would store both of them at once you could skip one validity execution.

Thanks for the suggestion. Originally, the methods used to have different parameters, I didn't notice they could be merged together after changing them, thanks for pointing this out!

Limeth · 2017-01-08T19:27:10Z

CraftingRecipe#getResult(GridInventory grid, World world) and getRemainingItems(GridInventory grid, World world) cannot be combined, because there are separate methods calling them in net.minecraft.item.crafting.CraftingManager.
I had to make a choice whether to make these two API methods return Optional<T>, where Optional#empty() would be returned when the recipe is not fulfilled, or just T without checking the recipe requirements. The downside of returning T directly is, that the API relies on that the user has checked the isValid method; on the other hand, if they decide to call both API methods, they only have to check the validity once.
~~Minecraft actually checks the recipe validity for both methods, so I decided to go with the Optional<T> return type.~~

Edit: Since users would have to implement IRecipe, the methods must basically be the same.

Cybermaxke · 2017-01-09T21:50:42Z

src/main/java/org/spongepowered/api/item/recipe/smelting/SmeltingRecipeRegistry.java

+ */
+package org.spongepowered.api.item.recipe.smelting;
+
+public interface SmeltingRecipeRegistry {


You forgot to extend the RecipeRegistry.

liach · 2017-01-10T03:00:19Z

src/main/java/org/spongepowered/api/item/recipe/smelting/SmeltingRecipe.java

+     * @return The amount of experience released after completing this recipe,
+     *         if the {@param ingredient} is valid
+     */
+    Optional<Double> getExperience(ItemStackSnapshot ingredient);


Use java.util.OptionalDouble

https://www.youtube.com/watch?v=PEZWYXPvmS8

@Limeth http://stackoverflow.com/a/33266445

But there are no map/flatMap functions for chaining in OptionalDouble. Is performance really at stake here?

Why do you have 2 wrappers if you can just have one @Limeth?

Exactly for the reason I stated above.

You don't really need to map for doubles.

liach is referring to .map(Function<T,R>).

~~Well if the absent result result is equivalent to "does not grant experience" then it could just return primitive double zero~~

liach · 2017-01-10T19:29:12Z

src/main/java/org/spongepowered/api/item/recipe/crafting/ShapedCraftingRecipe.java

+         * @param result The resultant snapshot
+         * @return The builder
+         */
+        Builder result(@Nullable ItemStackSnapshot result);


Same as above.

liach · 2017-01-10T19:29:24Z

src/main/java/org/spongepowered/api/item/recipe/crafting/ShapedCraftingRecipe.java

+         */
+        ShapedCraftingRecipe build();
+    }
+}


liach · 2017-01-10T19:29:34Z

src/main/java/org/spongepowered/api/item/recipe/crafting/ShapelessCraftingRecipe.java

+        return Sponge.getRegistry().createBuilder(Builder.class);
+    }
+
+


Extra empty lines

me4502 · 2017-01-12T01:28:49Z

This is more a question of curiosity, but does this support information such as Lore, Names, Enchantments, etc, on recipe items?

Limeth · 2017-01-13T11:49:54Z

@me4502 Yes, you can implement that ingredient predicate yourself and only return true if correct lore is present. I'm currently stuck implementing the SpongeCommon part of this PR, so if you anyone would like to help, that would be awesome.

liach · 2017-01-14T10:49:35Z

src/main/java/org/spongepowered/api/item/recipe/smelting/SmeltingRecipe.java

+         *                   is completed
+         * @return This builder, for chaining
+         */
+        Builder experience(@Nullable Double experience);


What does null stand for here? Use default?

liach · 2017-01-14T10:50:20Z

src/main/java/org/spongepowered/api/item/recipe/smelting/SmeltingRecipe.java

+         * @return This builder, for chaining
+         */
+        default Builder result(@Nullable ItemStack result) {
+            return result(result != null ? result.createSnapshot() : null);


Seems like you missed this one by not using ItemStackSnapshot.NONE.

ST-DDT · 2017-01-14T19:16:34Z

src/main/java/org/spongepowered/api/item/recipe/Recipe.java

-     */
-    Optional<List<ItemStack>> getResults(GridInventory grid);
-
+    ItemStackSnapshot getExemplaryResult();


Add an empty line after the last member in a class/interface.

ST-DDT · 2017-01-14T19:27:08Z

src/main/java/org/spongepowered/api/item/recipe/crafting/ShapedCraftingRecipe.java

+         * @param aisle A string array of ingredients
+         * @return The builder
+         */
+        Builder aisle(@Nullable String... aisle);


This method should mention what happens to the previously registered ingredient Predicates. (Especially those, that are no loner needed for this aisle.)

Ref: where(char, Predicate) <- @throws IllegalArgumentException If the aisle does not contain the specified character symbol

We should clarify what an 'asile' means here. If I'm correct in my understanding of it, each 'asile' corresponds to a row of the crafting grid.

ST-DDT · 2017-01-14T19:30:40Z

src/main/java/org/spongepowered/api/item/recipe/crafting/ShapedCraftingRecipe.java

+         * @throws IllegalArgumentException If the aisle does not contain
+         *     the specified character symbol
+         */
+        Builder where(char symbol, ItemStackSnapshot ingredient) throws IllegalArgumentException;


I recommend adding @Nullable here to keep the remove if null part. This should also document what happens if the ItemStackSnapshot.NONE is set (IMO its the same as null => remove ingredient )

ST-DDT · 2017-01-14T19:33:42Z

src/main/java/org/spongepowered/api/item/recipe/crafting/ShapedCraftingRecipe.java

+         */
+        @SuppressWarnings("ConstantConditions")
+        default Builder result(@Nullable ItemStack result) {
+            return result(result != null ? result.createSnapshot() : ItemStackSnapshot.NONE);


This method should document the implications of this ItemStackSnapshot.NONE here.
IMO an empty stack is not a valid result.

ST-DDT · 2017-01-14T19:35:42Z

src/main/java/org/spongepowered/api/item/recipe/crafting/ShapedCraftingRecipe.java

+         */
+        @SuppressWarnings("ConstantConditions")
+        default Builder where(char symbol, @Nullable ItemStack ingredient) throws IllegalArgumentException {
+            return where(symbol, ingredient != null ? ingredient.createSnapshot() : ItemStackSnapshot.NONE);


I'm not quite sure about the NONE part. because its effectifly an invalid ingredient.
Otherwise you have to document the implications of using this. Especially since the implications are slightly different than the other methods.

ST-DDT · 2017-01-14T19:58:36Z

src/main/java/org/spongepowered/api/item/recipe/smelting/SmeltingRecipe.java

+     * method, as it customizes the result further depending on the specified
+     * {@param ingredient} {@link ItemStackSnapshot}. It is advised to use
+     * the output of {@link #getExemplaryResult()}, modify it accordingly,
+     * and {@code return} it.


I'm not sure whether the main javadoc part should be about the implementation. I would recommend explaining when it is used.

Out of thin air example:

This method is called at the beginning of the smelting process to check whether there is space for the result and at the end of the smelting to calculate to actually add it to the results. <p>Note: It is advised that the result from getExemplaryResult and this method are strictly related to each other. / ... the results from this method are a derived result from getExemplaryResult </p>

ST-DDT · 2017-01-14T20:02:51Z

src/main/java/org/spongepowered/api/item/recipe/smelting/SmeltingRecipe.java

+         * @param result The output of this recipe
+         * @return This builder, for chaining
+         */
+        Builder result(@Nullable ItemStackSnapshot result);


What are the implications of setting null here?
IMO setting it to null is useless since you have to set it to something none null anyway.

ST-DDT · 2017-01-14T20:03:27Z

src/main/java/org/spongepowered/api/item/recipe/smelting/SmeltingRecipe.java

+         * @param ingredient The required ingredient
+         * @return This builder, for chaining
+         */
+        Builder ingredient(@Nullable ItemStackSnapshot ingredient);


Document null behaviour. See above.

ST-DDT · 2017-01-14T20:03:44Z

src/main/java/org/spongepowered/api/item/recipe/smelting/SmeltingRecipe.java

+         * @param ingredient The required ingredient
+         * @return This builder, for chaining
+         */
+        default Builder ingredient(@Nullable ItemStack ingredient) {


Document null behaviour. See above.

ST-DDT · 2017-01-14T20:05:20Z

src/main/java/org/spongepowered/api/item/recipe/smelting/SmeltingRecipe.java

+         */
+        SmeltingRecipe build();
+    }
+}


Missing newline + one a line above

ST-DDT · 2017-01-14T20:11:48Z

src/main/java/org/spongepowered/api/item/recipe/smelting/SmeltingRecipe.java

+     * Builds a simple furnace recipe
+     */
+    interface Builder extends ResettableBuilder<SmeltingRecipe, Builder> {
+        /**


Missing newline

ST-DDT

Good job. Almost done.

ST-DDT · 2017-01-14T22:53:33Z

src/main/java/org/spongepowered/api/item/recipe/crafting/CraftingRecipe.java

+     *
+     * <p>It is advised to use the output of
+     * {@link CraftingRecipe#getExemplaryResult()}, modify it accordingly,
+     * and {@code return} it.</p>


There should be a hint that this comment is only for implementing classes.

Implementing classes are advised...optionally cache it and {@code return} it.

ST-DDT · 2017-01-14T22:56:57Z

src/main/java/org/spongepowered/api/item/recipe/crafting/ShapedCraftingRecipe.java

+         * @return The builder
+         */
+        default Builder result(ItemStack result) {
+            return result(result.createSnapshot());


There should be a null check here checkNotNull(result, "result")

ST-DDT · 2017-01-14T22:58:17Z

src/main/java/org/spongepowered/api/item/recipe/crafting/ShapelessCraftingRecipe.java

+         * @return This builder, for chaining
+         */
+        default Builder result(ItemStack result) {
+            return result(result.createSnapshot());


Missing null check
return result(checkNotNull(result, "result").createSnapshot());

Maxqia

Ehhhhhh

Maxqia · 2017-01-15T02:32:34Z

src/main/java/org/spongepowered/api/item/recipe/crafting/CraftingRecipe.java

+     * @return {@code width * height} for shaped recipes, or the number of
+     *         ingredients for shapeless recipes
+     */
+    int getSize();


Invalid Description, It wouldn't do that if you added it after. Honestly this function isn't really necessary at all.

I believe the order defined in the implementation is so that the crafting manager can prefer large recipes (3x3) before considering smaller 2x2 ones.
This is somewhat implementation detail that need not be exposed.
For shaped recipes the implementation just needs to do getWidth() * getHeight() and for shapeless do getIngredientPredicates().size()

Maxqia · 2017-01-15T02:45:05Z

src/main/java/org/spongepowered/api/item/recipe/crafting/CraftingRecipe.java

+     * @param world The world this recipe would be used in
+     * @return True if the given input matches this recipe's requirements
+     */
+    boolean isValid(GridInventory grid, World world);


I'd say you should just combine this with getResult and default implement it to return false if it returns ItemTypes.AIR or null...

In order to support listing modded recipes, I had to make CraftingRecipe symmetric to IRecipe. Making this return the ItemStackSnapshot would also unnecessarily create instances in cases they aren't needed.

Maxqia · 2017-01-15T02:47:56Z

src/main/java/org/spongepowered/api/item/recipe/smelting/SmeltingRecipe.java

+     * @param ingredient The ingredient to check against
+     * @return Whether this {@param ingredient} can be used to craft the result
+     */
+    boolean isValid(ItemStackSnapshot ingredient);


Duplicate Function? and see above...

Not a duplicate. Extends Recipe, not CraftingRecipe.

Maxqia · 2017-01-15T02:50:36Z

src/main/java/org/spongepowered/api/item/recipe/smelting/SmeltingResult.java

+
+import org.spongepowered.api.item.inventory.ItemStackSnapshot;
+
+public interface SmeltingResult {


Why is this a separate interface?

In this case, we can afford to do the optimization you suggested for CraftingRecipe#isValid(GridInventory, World), because there is no special interface for smelting recipes in Minecraft code. This way, users don't get access to the resulting ItemStackSnapshot and the amount of experience released, unless the SmeltingRecipe#isValid(ItemStackSnapshot) returns true.

simon816 · 2017-01-15T20:06:46Z

src/main/java/org/spongepowered/api/item/recipe/crafting/CraftingRecipe.java

+     * Checks if the given {@link GridInventory} fits the required constraints
+     * to craft this {@link CraftingRecipe}.
+     *
+     * @param grid The ItemGrid to check for validity


ItemGrid was an old name, should now be GridInventory

simon816 · 2017-01-15T20:13:25Z

src/main/java/org/spongepowered/api/item/recipe/crafting/CraftingRecipe.java

+     * @param grid The crafting input, typically 3x3 or 2x2
+     * @return An {@link ItemStackSnapshot}
+     */
+    ItemStackSnapshot getResult(GridInventory grid);


Recipes can output multiple items
See this comment: #242 (comment)

Also, it's not defined what would happen if isValid returns false, does it throw an exception? Maybe it should return an Optional?

Crafting recipes cannot. That comment talks about pulverizer recipes.

What happens is undefined behavior, no way around that.

simon816 · 2017-01-15T20:16:02Z

src/main/java/org/spongepowered/api/item/recipe/crafting/CraftingRecipe.java

+
+    /**
+     * This method should only be called if
+     * {@link #isValid(GridInventory, World)} returns {@code true}.


Again, need to define what happens if isValid returns false

its not undefined it should throw an exception

Well, yes, I could add a line that the method might throw a specific exception, but it does not necessarily check the validity of the input crafting grid.

simon816 · 2017-01-15T20:19:44Z

src/main/java/org/spongepowered/api/item/recipe/smelting/SmeltingResult.java

+     * @return The result of fulfilling the requirements of a
+     *         {@link SmeltingRecipe}
+     */
+    ItemStackSnapshot getResult();


I think this should be a list of items. Modded recipes may return more than one item (e.g. a furnace that can smelt double at once)

Yes, but only in "modern" GUIs. Those recipes are not supported for standard vanilla GUIs as there is only one output slot.

Deamon5550 · 2017-01-16T22:41:33Z

src/main/java/org/spongepowered/api/item/recipe/smelting/SmeltingResult.java

+    private final double experience;
+
+    @SuppressWarnings("ConstantConditions")
+    public SmeltingResult(@Nonnull ItemStackSnapshot result, double experience) {


@Nonnull is not needed as things are nonnull by default

Aaron1011 · 2017-02-10T01:42:42Z

src/main/java/org/spongepowered/api/item/recipe/Recipe.java

+     * @return The exemplary result of this recipe
     */
-    Optional<List<ItemStack>> getResults(GridInventory grid);
+    ItemStackSnapshot getExemplaryResult();


I'm not sure if 'exemplary' makes sense here. What does this represent?

I think its the item you see in the CraftingOutput before you take it out.

The native IRecipe class contains an ItemStack getRecipeOutput(), where this value is used as the return value, and the FurnaceRecipes class contains a Map<ItemStack, ItemStack> smeltingList, where this value is used as a value in the map. I think these might be used in TooManyItems/NotEnoughItems to display the recipes, that's why I chose the word 'exemplary'.

Aaron1011 · 2017-02-10T01:48:02Z

src/main/java/org/spongepowered/api/item/recipe/crafting/ShapedCraftingRecipe.java

+         * @throws IllegalArgumentException If the aisle does not contain
+         *     the specified character symbol
+         */
+        Builder where(char symbol, @Nullable ItemStackSnapshot ingredient) throws IllegalArgumentException;


@Limeth: That link seems to be broken. Can the implementation take care of passing in ItemStackSnapshot.NONE to the predicate, if that's what the issue is?

Aaron1011 · 2017-02-10T01:48:54Z

src/main/java/org/spongepowered/api/item/recipe/crafting/CraftingRecipeRegistry.java

+    }
+
+    /**
+     * Creates a predicate with vanilla matching behavior.


Can you clarify what this means? Otherwise, plugins may not find it very useful.

Aaron1011 · 2017-02-10T01:51:38Z

src/main/java/org/spongepowered/api/item/recipe/crafting/ShapedCraftingRecipe.java

+         * @param aisle A string array of ingredients
+         * @return The builder
+         */
+        Builder aisle(@Nullable String... aisle);


We should clarify what an 'asile' means here. If I'm correct in my understanding of it, each 'asile' corresponds to a row of the crafting grid.

Limeth · 2017-02-10T12:26:20Z

@Aaron1011: You said, that That link seems to be broken. Can the implementation take care of passing in ItemStackSnapshot.NONE to the predicate, if that's what the issue is?
I couldn't reply directly to that comment for some reason. Anyway, I don't quite understand what you're pointing out here, could you elaborate?

Aaron1011 · 2017-02-10T19:48:52Z

@Limeth: What I'm wondering is this: Why do we need the overload of where that takes an ItemStackSnapshot (i.e. where(char symbol, @Nullable ItemStackSnapshot ingredient)). Is there something that can't be accomplished through passing in a Predicate?

Limeth · 2017-02-10T20:20:25Z

@Aaron1011 No, this is just a convenience method which uses the getVanillaIngredientPredicate you pointed out above. We don't need to overload it, but I'd find it useful.

stephan-gh · 2017-02-18T15:22:14Z

src/main/java/org/spongepowered/api/item/recipe/crafting/CraftingResult.java

+
+    @Override
+    public int hashCode() {
+        int result1 = this.result.hashCode();


You can use Objects.hash(this.result, this.remainingItems) here.

parlough · 2017-03-24T08:49:52Z

It seems like recipes and similar will change heavily in 1.12 with some changes Dinnerbone is working on, so it may be best to revisit this with that update.

gabizou · 2017-03-24T09:50:55Z

It seems like recipes and similar will change heavily in 1.12(or maybe 1.13) with some changes Dinnerbone is working on, so it may be best to revisit this with that update.

I'd rather have recipes partially implemented for API 6 and 7 while we still can, if not fully implemented, so that when MC 1.12 comes around, we can easily update the API as necessary, along with the implementation.

Faithcaio · 2017-06-12T13:02:58Z

MC 1.12 is out and Sponge still has no working Recipe API.
What is holding this back?

Zidane · 2017-06-12T14:46:38Z

@Faithcaio @gabizou Not really interested in adding work back for older versions. Now that Forge 1.12 is in the wild, mods will skip 1.11 and head straight for that and leave 1.10 behind. This API needs to be updated for 1.12's changes and proceed from there.

parlough · 2017-06-12T14:48:09Z

It seems he has a few changes to make here still but especially in the implementation with there being suggestions, questions, merge conflicts, and I'm assuming issues with 1.12 changes. @Limeth do you have the time and/or interest to continue these pull requests?

Limeth · 2017-06-12T16:21:53Z

I got discouraged by the PR process and am no longer interested in contributing. If anyone wants to take over this PR, feel free to do so.

liach · 2017-06-12T16:41:56Z

Yeah Sponge devs often respond slowly.

kashike · 2017-06-12T16:55:49Z

@Limeth May I ask what discouraged you, and why you're no longer interested in contributing?

Limeth · 2017-06-12T17:19:20Z

@kashike It was difficult for me to adapt to the required code style, most of the responses on my PRs were only addressing the "distracting" parts of code that didn't follow the code style. This lead to much longer periods of the iterative development process (code/compile/test/debug), as I felt like I didn't get enough feedback on the actual logic I was implementing.
Along with this, I got busy with school and when I returned, I realized programming Minecraft plugins wasn't what I'd like to continue on doing, at least for now.

jamierocks · 2017-06-12T21:23:20Z

As it has been brought up, I thought I would share https://dev.to/terrehbyte/my-pull-request-etiquette - I thought it was a rather good read, and although not all of it would be easy for Sponge to implement, I'm sure it'd be worth some though ;)

There are a number of other good reads over on dev.to regarding code reviews, such as: https://dev.to/vaidehijoshi/crafting-better-code-reviews and https://dev.to/cyberomin/a-pull-request-is-submitted-what-next

kashike · 2017-06-15T12:57:48Z

Superseded by #1582

ST-DDT suggested changes Jan 2, 2017

View reviewed changes

Deamon5550 reviewed Jan 2, 2017

View reviewed changes

kashike suggested changes Jan 2, 2017

View reviewed changes

Limeth mentioned this pull request Jan 3, 2017

Generalize recipes and implement them SpongePowered/Sponge#1119

Closed

6 tasks

Deamon5550 suggested changes Jan 3, 2017

View reviewed changes

ST-DDT suggested changes Jan 7, 2017

View reviewed changes

Cybermaxke reviewed Jan 9, 2017

View reviewed changes

liach reviewed Jan 10, 2017

View reviewed changes

gabizou mentioned this pull request Jan 10, 2017

Refactor recipes #1098

Closed

Limeth mentioned this pull request Jan 14, 2017

Generalize recipes and implement them SpongePowered/SpongeForge#1188

Closed

liach reviewed Jan 14, 2017

View reviewed changes

ST-DDT suggested changes Jan 14, 2017

View reviewed changes

ST-DDT reviewed Jan 14, 2017

View reviewed changes

ST-DDT suggested changes Jan 14, 2017

View reviewed changes

Maxqia reviewed Jan 15, 2017

View reviewed changes

simon816 suggested changes Jan 15, 2017

View reviewed changes

Deamon5550 reviewed Jan 16, 2017

View reviewed changes

Limeth and others added 9 commits February 8, 2017 15:29

minor adjustments

5d85b69

added a findMatchingRecipe method

124a286

implement smelting recipes

63b7ac6

PR adjustments

b84a8a7

remove unnecessary @nonnulls

de99fc1

No star imports.

8f7d40b

make getIngredientPredicate throw an exception if out of bounds

0ef092b

expose various methods in recipe registries; reintroduce CraftingResult

40b6241

implement equals/hashCode/toString in recipe result classes

ea6161b

Limeth force-pushed the feature/recipe-ii branch from d29c46e to ea6161b Compare February 8, 2017 14:29

Aaron1011 suggested changes Feb 10, 2017

View reviewed changes

improve javadocs

2b0ba2d

stephan-gh reviewed Feb 18, 2017

View reviewed changes

malte-laukoetter mentioned this pull request Apr 22, 2017

Add CraftItem to Possible actions malte-laukoetter/Laborus#8

Open

Faithcaio mentioned this pull request Jun 15, 2017

Recipe API #1582

Closed

11 tasks

kashike closed this Jun 15, 2017


		import org.spongepowered.api.item.inventory.ItemStackSnapshot;

		public interface SmeltingResult {

Uh oh!

Conversation

Limeth commented Jan 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Limeth commented Jan 2, 2017

Uh oh!

ST-DDT Jan 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabizou Jan 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Aaron1011 Feb 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ST-DDT Jan 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Deamon5550 Jan 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Maxqia commented Jan 3, 2017

Uh oh!

Limeth commented Jan 3, 2017

Uh oh!

Deamon5550 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Limeth commented Jan 2, 2017 •

edited

Loading

ST-DDT Jan 2, 2017 •

edited

Loading

gabizou Jan 10, 2017 •

edited

Loading

Aaron1011 Feb 10, 2017 •

edited

Loading

ST-DDT Jan 2, 2017 •

edited

Loading

Deamon5550 Jan 2, 2017 •

edited

Loading

ST-DDT Jan 7, 2017 •

edited

Loading